CMS_PERMISSION: optionally use raw id user lookups #1481

jimr · 2012-10-17T16:31:05Z

The "view restrictions" and "page permissions" inlines on the "page"
admin change forms can cause performance problems where there are many
thousands of users being put into simple select boxes. Added a new
setting (CMS_RAW_ID_USERS) that, if set, forces the inlines on that page
to use standard Django admin "raw ID" lookups rather than select boxes.
The restriction on which users may be selected should still be honoured
by the new widget because we set the 'limit_choices_to' attribute to be
the same as the queryset on the original widget.

Using raw_id_fields in combination with limit_choices_to blows up if you
have many thousands of users. For this reason, we only apply this limit
if the number of users is relatively small (< 500, though that figure is
somewhat arbitrary). If the number of users we need to limit to is
greater than that, we use the usual input field instead unless the user
is a CMS superuser, in which case we bypass the limit. Unfortunately,
this means that non-superusers won't see any benefit from this change.

The "view restrictions" and "page permissions" inlines on the "page" admin change forms can cause performance problems where there are many thousands of users being put into simple select boxes. Added a new setting (CMS_RAW_ID_USERS) that, if set, forces the inlines on that page to use standard Django admin "raw ID" lookups rather than select boxes. The restriction on which users may be selected should still be honoured by the new widget because we set the 'limit_choices_to' attribute to be the same as the queryset on the original widget. Using raw_id_fields in combination with limit_choices_to blows up if you have many thousands of users. For this reason, we only apply this limit if the number of users is relatively small (< 500, though that figure is somewhat arbitrary). If the number of users we need to limit to is greater than that, we use the usual input field instead unless the user is a CMS superuser, in which case we bypass the limit. Unfortunately, this means that non-superusers won't see any benefit from this change.

jimr · 2012-10-17T16:33:17Z

As mentioned on the developers mailing list: https://groups.google.com/forum/#!topic/django-cms-developers/zprGZo6Pz1Y

digi604 · 2012-10-17T19:33:23Z

Ok this needs docs and it needs merge with develop. I don't think we gonna release an other 2.2 release for this.

jimr · 2012-10-18T08:39:51Z

OK, I'll add some docs & remove the version bump.

Also removed the previous version bump so it can merge cleanly upstream.

digi604 · 2012-11-12T13:00:41Z

Looks good to me... @ojii could you have a look as well?

ojii · 2012-11-12T13:02:37Z

cms/admin/forms.py

+
+        limit_choices = True
+        use_raw_id = False
+        if hasattr(settings, 'CMS_RAW_ID_USERS') and settings.CMS_RAW_ID_USERS:


if getattr(settings, 'CMS_RAW_ID_USERS', False):

Yes, that would be clearer.

ojii · 2012-11-12T13:03:43Z

I have a few isuses with the patch itself, but on top of that I'm (as we all know by now) not a huge fan of adding new settings.

If this is such a good thing, why don't we just make it the default?

digi604 · 2012-11-12T13:41:38Z

Good point @ojii. Remove the setting and make it an Raw ID field if there are more 500 users? Maybe even 200?

jimr · 2012-11-12T14:08:45Z

I only made it a setting because I didn't want to alter default behaviour. With thousands of user accounts, it's currently infeasible to use CMS_PERMISSION because of the issues addressed by this patch, so making it default would make sense to me.

digi604 · 2013-03-28T16:52:21Z

please remerge with develop and i think too that the configurable limit would be better...

ojii · 2013-03-29T05:51:43Z

cms/conf/global_settings.py

@@ -26,6 +26,9 @@
 # Whether to enable permissions.
 CMS_PERMISSION = False

+# Whether to use raw ID lookups for users when CMS_PERMISSION is set to True
+CMS_RAW_ID_USERS = False


with regards to the limit, how about making this setting either False (disable the raw ID lookup) or an integer which is the limit?

jimr · 2013-04-02T08:48:47Z

OK, sounds good. I'll bring my branch up to date & make the limit configurable.

jimr · 2013-04-05T09:47:33Z

@ojii should I put CMS_RAW_ID_USERS in DEFAULTS in cms/utils/conf.py as RAW_ID_USERS? Seems the arrangement of settings has changed rather a lot since I started.

ojii · 2013-04-05T09:49:32Z

yes please

Conflicts: cms/conf/global_settings.py

So the RAW_ID_USERS setting should now be either left as False or set to a positive integer which will be the threshold for the number of users above which the raw ID widget will be used.

ojii · 2013-04-05T13:34:59Z

cms/admin/permissionadmin.py

@@ -27,6 +27,16 @@ class PagePermissionInlineAdmin(TabularInline):
    exclude = ['can_view']
    extra = 0 # edit page load time boost

+    def __getattribute__(self, name):


github seems to have lost this discussion, but this works for me:

In [1]: class A: ...: @property ...: def b(self): ...: return [] ...: In [2]: a = A() In [3]: isinstance(a.b, list) Out[3]: True In [4]: isinstance(a.b, (list, tuple)) Out[4]: True

Django checks it on the class, not an instance, otherwise it would indeed work.

https://github.com/django/django/blob/master/django/contrib/admin/validation.py#L267

class classproperty(object): def __init__(self, fget): self.fget = fget def __get__(self, owner_self, owner_cls): return self.fget(owner_cls) ## -- End pasted text -- In [2]: class A: ...: @classproperty ...: def b(cls): ...: return [] ...: In [3]: A.b Out[3]: []

OK, so that would work, but do you think it's better than just using __getattribute__? Up to you, but IMO the original code is easier to understand.

arguably, but I just really am not a fan of __getattribute__. had too many issues with it in the past...

Fair enough. Let me know if this is blocking the merge and I'll change it, but my personal preference would be to leave as-is.

had a chat with @digi604 and @stefanfoulis and we all three prefer classproperty over __getattribute__.

I think the best place to put that helper class is probably cms.utils.helpers, so if you could change this bit to make use of that technique, that would be highly appreciated.

Added the new classproperty helper in cms.utils.helpers.

CMS_PERMISSION: optionally use raw id user lookups

jimr · 2013-04-15T15:37:49Z

Thanks!

digi604 · 2013-04-15T15:42:00Z

Thank YOU!

James Rutherford added 3 commits September 14, 2012 10:28

Removed unused import

1ea6e55

.count() rather than len() for performance boost

2b1b614

CMS_RAW_ID_USERS: added default setting + docs

b3183f2

Also removed the previous version bump so it can merge cleanly upstream.

ojii reviewed Nov 12, 2012
View reviewed changes

clearer conditional check

5e823d5

ojii reviewed Mar 29, 2013
View reviewed changes

James Rutherford added 3 commits April 5, 2013 14:14

Merge branch 'develop' into raw_id_users

2481bf3

Conflicts: cms/conf/global_settings.py

Only use raw ID above a certain number of users

ec9f8d8

So the RAW_ID_USERS setting should now be either left as False or set to a positive integer which will be the threshold for the number of users above which the raw ID widget will be used.

import typo

61055b9

ojii reviewed Apr 5, 2013
View reviewed changes

Switch from __getattribute__ to classproperty

9a778f5

Added the new classproperty helper in cms.utils.helpers.

digi604 added a commit that referenced this pull request Apr 15, 2013

Merge pull request #1481 from Maplecroft/raw_id_users

bd166e3

CMS_PERMISSION: optionally use raw id user lookups

digi604 merged commit bd166e3 into django-cms:develop Apr 15, 2013

jimr deleted the raw_id_users branch April 15, 2013 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CMS_PERMISSION: optionally use raw id user lookups #1481

CMS_PERMISSION: optionally use raw id user lookups #1481

jimr commented Oct 17, 2012

jimr commented Oct 17, 2012

digi604 commented Oct 17, 2012

jimr commented Oct 18, 2012

digi604 commented Nov 12, 2012

ojii Nov 12, 2012

jimr Nov 12, 2012

ojii commented Nov 12, 2012

digi604 commented Nov 12, 2012

jimr commented Nov 12, 2012

digi604 commented Mar 28, 2013

ojii Mar 29, 2013

jimr commented Apr 2, 2013

jimr commented Apr 5, 2013

ojii commented Apr 5, 2013

ojii Apr 5, 2013

jimr Apr 5, 2013

ojii Apr 5, 2013

jimr Apr 5, 2013

ojii Apr 5, 2013

jimr Apr 5, 2013

ojii Apr 6, 2013

jimr Apr 8, 2013

jimr commented Apr 15, 2013

digi604 commented Apr 15, 2013

CMS_PERMISSION: optionally use raw id user lookups #1481

CMS_PERMISSION: optionally use raw id user lookups #1481

Conversation

jimr commented Oct 17, 2012

jimr commented Oct 17, 2012

digi604 commented Oct 17, 2012

jimr commented Oct 18, 2012

digi604 commented Nov 12, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ojii commented Nov 12, 2012

digi604 commented Nov 12, 2012

jimr commented Nov 12, 2012

digi604 commented Mar 28, 2013

Choose a reason for hiding this comment

jimr commented Apr 2, 2013

jimr commented Apr 5, 2013

ojii commented Apr 5, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimr commented Apr 15, 2013

digi604 commented Apr 15, 2013